tests(smokehouse): fail on finalUrl/errorCode mismatches #7227
Conversation
connorjclark
requested review from
brendankenny and
patrickhulce
as code owners
|
|
||
| let correctCount = 0; | ||
| let failedCount = 0; | ||
| results.audits.forEach(auditAssertion => { |
There was a problem hiding this comment.
| results.audits.forEach(auditAssertion => { | |
| [results.finalUrl, results.errorCode, ...results.audits].forEach(auditAssertion => { |
There was a problem hiding this comment.
could do this instead and drop the two reportAssertions above.
There was a problem hiding this comment.
nice
do we want to continue showing green ticks for passing? this is what it looks like with those changes + moving reportAssertion out of the if statement:
Keeping reportAssertion where it is-
There was a problem hiding this comment.
i like this best
brendankenny
changed the title
There was a problem hiding this comment.
I personally prefer the shorter version
It's more compact, which is nice when looking at CI logs (we have 17 sets of expectations now), you get the failed assertion and diff if there was a failure, and you can always look in the individual expectations files if you need to see what all the passing tests were (we can make a --verbose flag to print all of them, but I doubt anyone will ever use it).
brendankenny
changed the title
|
I kinda like the verbose version. My most common use case when actually running smoketests is picking a particular smoke test and running it frequently so I kinda like the ability to see what's passing. I have no strong feelings on the length of passing CI logs since those are collapsed for me anyhow :) |
|
|
|
Went with an env flag since wiring an option would be more work (see how run-smokehouse parses argv). |
| @@ -35,6 +35,7 @@ const PAGE_HUNG_EXIT_CODE = 68; | |||
| const INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; | |||
| const RETRIES = 3; | |||
| const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/; | |||
| const VERBOSE = process.env.LH_SMOKE_VERBOSE === '1'; | |||
There was a problem hiding this comment.
WDYT about just Boolean(process.env.LH_SMOKE_VERBOSE)?
There was a problem hiding this comment.
later down we use a env var SMOKEHOUSE_DEBUG maybe we should unify these two 🤔
maybe we rename that one to LH_SMOKE_DEBUG?
| @@ -276,8 +277,12 @@ function reportAssertion(assertion) { | |||
| RegExp.prototype.toJSON = RegExp.prototype.toString; | |||
|
|
|||
| if (assertion.equal) { | |||
| console.log(` ${log.greenify(log.tick)} ${assertion.category}: ` + | |||
| log.greenify(assertion.actual)); | |||
| if (assertion.actual !== null && typeof assertion.actual === 'object') { | |||
There was a problem hiding this comment.
super nit but mind flipping the order of these to is an object && is not null? took me a bit to realize this was just an object check
There was a problem hiding this comment.
can we just use a helper method like so?
const isPlainObject = function (obj) {
return Object.prototype.toString.call(obj) === '[object Object]';
};There was a problem hiding this comment.
Object.prototype.toString.call(obj) === '[object Object]';
what year is it?
| @@ -62,7 +63,7 @@ function resolveLocalOrCwd(payloadPath) { | |||
| * @return {ExpectedLHR} | |||
| */ | |||
| function runLighthouse(url, configPath, isDebug) { | |||
| isDebug = isDebug || Boolean(process.env.SMOKEHOUSE_DEBUG); | |||
| isDebug = isDebug || Boolean(process.env.LH_SMOKE_DEBUG); | |||
There was a problem hiding this comment.
doesn't debug also save the LHR? Up to y'all if you want that :)
There was a problem hiding this comment.
nah. just writing to disk is good enough for me. verbose shouldn't be too verbose
Co-Authored-By: Hoten <cjamcl@gmail.com>
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
Fixes #7225.